-
Notifications
You must be signed in to change notification settings - Fork 154
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sqlite database now gets the session_timestamp from the history session id #674
Conversation
CI is mad but should be an easy fix with rustfmt. When I originally wrote the sqlite sessions stuff, I included the ability to toggle the session. I'm wondering if that would be useful for what you're trying to do here. This is the original PR #562. Look for |
I have taken a look at I did run the cargo fmt and clippy commands but for some reason a lot of changes happened and I don't know why. Same with the one commit (nushell/nushell@2dcb821) in nushell. |
We probably won't be able to accept this the way it is, marking this draft for now. My guess is you have a rustfmt.toml somewhere that is influencing how it runs. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #674 +/- ##
=======================================
Coverage 49.19% 49.19%
=======================================
Files 46 46
Lines 7930 7930
=======================================
Hits 3901 3901
Misses 4029 4029
|
I've been running the command suggested on the Contributing part of the reedline repository' readme in the same directory where I cloned the reedline repository. I don't know why it acted out like this. If you have any idea how I could troubleshoot this, that would be great. |
@dasbard My only guess is that you have one of these two things going on.
I haven't seen a user have this problem before so I'm not filled with suggestions. Hope you can figure it out. |
Turns out I had a rustfmt.toml file inside my home directory for 4 months! I have looked into my~/ .cargo folder before and couldn't find anything and somehow missed that rustfmt.toml could also just be inside ~/ Maybe this could be avoided in the future with an empty rustfmt.toml file inside the repo overwriting user specific format files? |
Revert "cargo fmt + clippy fix (besides cargo fmt internal errors that don't reside from my changes)" This reverts commit a8c0682.
Things should finally be fixed. I've also removed the update_session function for the History trait because that seems to not get anywhere and currently has no known usecase as far as I can think. |
Are there any tests you can write that run in our CI to ensure this feature doesn't get broken in the future? Also there are conflicts. |
I'm unsure about some tests. Maybe I should take a look at it. |
it looks like you need to pull the latest main branch and then git merge main in this branch and resolve the conflicts there, then push to the pr again. |
It's been a while and I'm lost as to what problem this PR is fixing. Can you talk more about that and fix the conflicts @dasbard? |
unresponsive |
(Sub pull request required for a nushell pull request)
Obtains the
session_timestamp
for theSqliteBackedHistory
from thesession
(HistorySessionId
) to avoid desynchronization.The commented out
update_session
function on theHistory
trait will be explained in the other nushell pull request.